fix: do not double hash in secp256r1-verify
#6763
Open
+323
−8
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a consensus breaking change and will need to go out in a hard-fork with a new version of Clarity (Clarity5). In the existing implementation, the code was incorrectly hashing the passed in `message-hash`. Unfortunately, the test code was also double-hashing on the signing side as well, so this was not caught. The bug was discovered by @eric-stacks when attempting to generate signatures externally (e.g. with WebAuthn) and verify them in Clarity code.
Codecov Report❌ Patch coverage is
❌ Your project check has failed because the head coverage (66.83%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.
Additional details and impacted files@@ Coverage Diff @@
## develop #6763 +/- ##
============================================
- Coverage 78.18% 66.83% -11.35%
============================================
Files 580 585 +5
Lines 361096 361965 +869
============================================
- Hits 282312 241918 -40394
- Misses 78784 120047 +41263
... and 317 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Contributor
|
I think there's some test vectors for secp256r1 produced by NIST -- see the test vectors section at the bottom here (https://csrc.nist.gov/Projects/cryptographic-algorithm-validation-program/Component-Testing): Not sure if these would have caught the fact that we're double hashing. |
Contributor
|
Yeah -- I think we probably would have caught this if we used the NIST test vectors: diff --git a/clarity/src/vm/tests/crypto.rs b/clarity/src/vm/tests/crypto.rs
index 05d9460320..842090421e 100644
--- a/clarity/src/vm/tests/crypto.rs
+++ b/clarity/src/vm/tests/crypto.rs
@@ -3,13 +3,81 @@ use clarity_types::VmExecutionError;
use proptest::prelude::*;
use stacks_common::types::chainstate::{StacksPrivateKey, StacksPublicKey};
use stacks_common::types::{PrivateKey, StacksEpochId};
-use stacks_common::util::hash::{to_hex, Sha256Sum};
+use stacks_common::util::hash::{Sha256Sum, hex_bytes, to_hex};
use stacks_common::util::secp256k1::MessageSignature as Secp256k1Signature;
-use stacks_common::util::secp256r1::{Secp256r1PrivateKey, Secp256r1PublicKey};
+use stacks_common::util::secp256r1::{MessageSignature, Secp256r1PrivateKey, Secp256r1PublicKey};
use crate::vm::types::{ResponseData, TypeSignature, Value};
use crate::vm::{execute_with_parameters, ClarityVersion};
+struct NistVector {
+ msg: &'static str,
+ d: &'static str,
+ q_x: &'static str,
+ q_y: &'static str,
+ r: &'static str,
+ s: &'static str,
+}
+
+static NIST_VECTORS: &[NistVector] = &[
+ NistVector {
+ msg: "44acf6b7e36c1342c2c5897204fe09504e1e2efb1a900377dbc4e7a6a133ec56",
+ d: "519b423d715f8b581f4fa8ee59f4771a5b44c8130b4e3eacca54a56dda72b464",
+ q_x: "1ccbe91c075fc7f4f033bfa248db8fccd3565de94bbfb12f3c59ff46c271bf83",
+ q_y: "ce4014c68811f9a21a1fdb2c0e6113e06db7ca93b7404e78dc7ccd5ca89a4ca9",
+ r: "f3ac8061b514795b8843e3d6629527ed2afd6b1f6a555a7acabb5e6f79c8c2ac",
+ s: "8bf77819ca05a6b2786c76262bf7371cef97b218e96f175a3ccdda2acc058903",
+ }
+];
+
+#[rstest]
+#[case(0)]
+fn secp256r1_verify_valid_signatures_nist(
+ #[case] nist_index: usize
+) {
+ let NistVector { msg, d, q_x, q_y, r, s } = NIST_VECTORS[nist_index];
+
+ let message_hash = hex_bytes(msg).unwrap();
+ let privk = Secp256r1PrivateKey::from_hex(d).unwrap();
+ let mut pubk = Secp256r1PublicKey::from_hex(&format!("04{q_x}{q_y}")).unwrap();
+ pubk.set_compressed(true);
+
+ let signature_dh = privk.sign(&message_hash).unwrap();
+ let signature_nist = MessageSignature::from_hex(&format!("{r}{s}")).unwrap();
+
+ let verified_double_hash = pubk.verify(&message_hash, &signature_dh).is_ok();
+ let verified_nist_sign = pubk.verify_digest(&message_hash, &signature_nist).is_ok();
+
+ info!(
+ "Signed with SK";
+ "nist_signature" => format!("{r}{s}"),
+ "double_hash" => to_hex(&signature_dh.0),
+ "pubk" => to_hex(&pubk.to_bytes()),
+ "verified_double_hash" => verified_double_hash,
+ "verified_nist_sign" => verified_nist_sign,
+ );
+
+ let program = format!(
+ "(secp256r1-verify {} {} {})",
+ buff_literal(&message_hash),
+ buff_literal(&signature_nist.0),
+ buff_literal(&pubk.to_bytes())
+ );
+
+ assert_eq!(
+ Value::Bool(true),
+ execute_with_parameters(
+ program.as_str(),
+ ClarityVersion::Clarity4,
+ StacksEpochId::Epoch33,
+ false
+ )
+ .expect("execution should succeed")
+ .expect("should return a value")
+ );
+}
+
+
fn secp256r1_vectors() -> (Vec<u8>, Vec<u8>, Vec<u8>) {
let privk = Secp256r1PrivateKey::from_seed(&[7u8; 32]);
let pubk = Secp256r1PublicKey::from_private(&privk);That test fails: But you can see that it would have passed with pre-hash verification. |
Contributor
Author
|
Thanks for checking that Aaron. Want to just push that test to this branch? |
Contributor
Author
|
I added in them 8b8451a. |
aaronb-stacks
previously approved these changes
Dec 19, 2025
|
Thxs @eric-stacks for discovering this. |
|
Related for your consideration @obycode https://github.com/Rapha-btc/csw-locker-contracts/blob/main/readme-webauthn.md also fyi summary: WebAuthn signature verification for Clarity 5+ |
jcnelson
previously approved these changes
Jan 8, 2026
|
|
||
| /// Sign a message hash, returning the signature. | ||
| /// Sign a message hash, SHA256 hashing it (i.e. double-hashing) before | ||
| /// returning the signature. |
Member
There was a problem hiding this comment.
We may want to explicitly note that the sign() and verify() functions have been (or will be) superseded by the sign_digest() and verify_digest() functions. Based on the code above, I don't think we intend to expose the old double-SHA256 behavior in Clarity 5, right?
Contributor
Author
There was a problem hiding this comment.
Right, it will not be exposed in Clarity 5, but it needs to continue to exist for Clarity 4 contracts.
brice-stacks
dismissed stale reviews from jcnelson and aaronb-stacks
via
282a71d
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a consensus breaking change and will need to go out in a hard-fork with a new version of Clarity (Clarity5). In the existing implementation, the code was incorrectly hashing the passed in
message-hash. Unfortunately, the test code was also double-hashing on the signing side as well, so this was not caught. The bug was discovered by @eric-stacks when attempting to generate signatures externally (e.g. with WebAuthn) and verify them in Clarity code. The docs are updated to match the actual behavior and will need to be updated again once Clarity 5 is added.